Skip to content
This repository was archived by the owner on Dec 19, 2018. It is now read-only.

Extra newline no longer rendered at the end of a code block#507

Merged
ajaybhargavb merged 1 commit intodevfrom
ajbaaska/removewhitespace.485
Sep 10, 2015
Merged

Extra newline no longer rendered at the end of a code block#507
ajaybhargavb merged 1 commit intodevfrom
ajbaaska/removewhitespace.485

Conversation

@ajaybhargavb
Copy link
Copy Markdown
Contributor

Issue - #485

This fixes the issue with the first example in #485 (comment).
The second example no longer seems to be an issue.
I'm trying various scenarios to see if that issue exists and also to see if this change broke something else. (All tests passing other than the ones updated here)

@NTaylorMullen @dougbu

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add @{ } <html>\r\n variation to ensure that the end newline isn't ignored.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@NTaylorMullen
Copy link
Copy Markdown

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/removewhitespace.485 branch 2 times, most recently from dfd3072 to 743df7a Compare September 8, 2015 22:24
@ajaybhargavb
Copy link
Copy Markdown
Contributor Author

Updated.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you always complete the block here whether it's nested or not?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the nested situation we're not nulling out the SpanKind.Code's ChunkGenerator. I think this should be fine but we should validate with debugging (make sure not excess whitespace is highlighted) once a WTE is produced.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. The name is misleading here. This CompleteBlock does nothing here other than EnsureCurrent and PutCurrentBack. Removing it affects no tests. I am thinking we should remove it completely.

@NTaylorMullen
Copy link
Copy Markdown

@ajaybhargavb
Copy link
Copy Markdown
Contributor Author

Updated.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more tests

  • @{\r\n} \t\r\n<html>
  • @{\r\n@if(true){\r\n} <input> }
  • @{\r\n@if(true){\r\n} \r\n<input> \r\n}<html>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@NTaylorMullen
Copy link
Copy Markdown

@ajaybhargavb
Copy link
Copy Markdown
Contributor Author

Updated.

@NTaylorMullen
Copy link
Copy Markdown

:shipit:

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/removewhitespace.485 branch from 7aa7d84 to 5ef10af Compare September 10, 2015 17:37
- #485
- Using a flag to consume whitespace and newline at the end of a verbatim block
- Added tests to validate nested blocks
@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/removewhitespace.485 branch from 5ef10af to e2881b0 Compare September 10, 2015 17:39
@ajaybhargavb ajaybhargavb merged commit e2881b0 into dev Sep 10, 2015
@NTaylorMullen NTaylorMullen deleted the ajbaaska/removewhitespace.485 branch September 24, 2015 17:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants